-
Notifications
You must be signed in to change notification settings - Fork 786
Add current indexed version to repos table. #1262
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
can you fix tests @amotzte ? thnx (check travis - https://travis-ci.org/OpenGrok/OpenGrok/builds/172456917 )
|
also - and this is a prereq - we will need to cache this somehow to history folder, I don't think it is good to query SCM for every web request |
I agree with OCA for this patch Hi @tarzanek as far as I understand, all of the table information (Including this new field) is cached. The determineCurrentVersion is being called in only 2 cases:
The only function that is being called for every request is getCurrentVersion. This function returns the value from a local variable. Am I missing something? |
@@ -263,8 +264,13 @@ Copyright (c) 2015, 2016, Oracle and/or its affiliates. All rights reserved. | |||
if (branch == null) { | |||
branch = "N/A"; | |||
} | |||
String currentVersion = ri.getCurrentVersion(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two places in repos.jspf
where a line about repository is being printed to the output. So this should be handled in both cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@Override | ||
String determineCurrentVersion() { | ||
return null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong copyright date.
@@ -369,4 +369,9 @@ String determineParent() throws IOException { | |||
String determineBranch() { | |||
return null; | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong copyright date.
@@ -418,4 +418,9 @@ String determineParent() throws IOException { | |||
String determineBranch() { | |||
return null; | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong copyright date.
@@ -681,4 +681,9 @@ String determineParent() throws IOException { | |||
|
|||
return executor.getOutputString().trim(); | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong copyright date.
@@ -364,6 +364,8 @@ final void createCache(HistoryCache cache, String sinceRevision) | |||
*/ | |||
abstract String determineBranch() throws IOException; | |||
|
|||
abstract String determineCurrentVersion() throws IOException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong copyright date.
@@ -42,6 +42,7 @@ | |||
protected String datePattern; | |||
protected String parent; | |||
protected String branch; | |||
protected String currentVersion; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong copyright date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -292,4 +292,9 @@ String determineParent() throws IOException { | |||
String determineBranch() { | |||
return null; | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong copyright date.
@@ -400,4 +400,9 @@ String determineParent() throws IOException { | |||
String determineBranch() { | |||
return null; | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong copyright date.
@@ -364,6 +364,8 @@ final void createCache(HistoryCache cache, String sinceRevision) | |||
*/ | |||
abstract String determineBranch() throws IOException; | |||
|
|||
abstract String determineCurrentVersion() throws IOException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why it's made abstract when you return null
in all repositories except GitRepository
. This is also about the other determineBranch
and determineParent
methods. @tarzanek
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point... I followed the determineBranch function (without thinking about it) but you are absolutely right...
%><td><%= Util.htmlize(type) %></td><% | ||
%><td><%= Util.htmlize(parent) %> (<%= Util.htmlize(branch) %>)</td><% | ||
%><td><%= Util.htmlize(currentVersion) %> </td><% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the commit message is quite long (which sometimes happen) then the table looks ugly - I'd rather see it on a single line.
I'd suggest something like displaying the date, changeset, author and first like 20-30 characters from the message (maybe with an option to show the whole message like it is in history view) + convert newlines to spaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%><td><%= Util.htmlize(type) %></td><% | ||
%><td><%= Util.htmlize(parent) %> (<%= Util.htmlize(branch) %>)</td><% | ||
%><td><%= Util.htmlize(currentVersion) %> </td><% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My output does not match your screenshot. Especially I don't like the enclosing '.
If you want to reformat the date to something cooler, take a look at default date format for git in history generation and getting the date formatter for git repository.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@amotzte hmm, I was more like thinking about the situation where the history is not stored in javadb and just uses plain files - will it cache the data even in this case? ;) |
@tarzanek All field (branch, parent scm...) in the "homepage" table are stored in projects configuration configuration.xml. Here is an example: <void method="add">
<object class="org.opensolaris.opengrok.history.RepositoryInfo">
<void property="branch">
<string>master</string>
</void>
<void property="currentVersion"> <!-- <-- this is the new addition -->
<string>2016-09-04 17:00: 638bcfb mregev Merge pull request #2 from SPVSS-IH-UI/ui-server-ha</string>
</void>
<void property="datePattern">
<string>EE, d MMM yyyy HH:mm:ss Z</string>
</void>
<void property="directoryName">
<string>/var/opengrok/src/vgw_tests_and_deployment/ih-ui-server</string>
</void>
<void property="parent">
<string>git@github3.cisco.com:SPVSS-IH-UI/ih-ui-server.git</string>
</void>
<void property="type">
<string>git</string>
</void>
</object>
</void> So to my understanding all of these fields are cached/stored (in RuntimeEnvironment?) so we will not be executing git command for each web request in the same way that are are not doing it now for the rest of these fields . |
%><td><%= Util.htmlize(type) %></td><% | ||
%><td><%= Util.htmlize(parent) %> (<%= Util.htmlize(branch) %>)</td><% | ||
%><td><% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to set the table column to be at fixed size so in case user choose to open the "show more..." it will not change the other columns size (which feel annoying in the eye..). Currently they are divided with default size (each column get the exact some size). I guess I can reduce the size of "SCM type" to something smaller but still fixed and give more space to "Parent (branch)" and "Current version". I'll give it a try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm more talking about this, which is not like it was before (there was just single line with no margin). It's solved for me by replacing <p>
with <span>
however then the "show more" link does not work.
You can restrict the scm column to some small percentage of the width, and also you always know what is the longest scm supported by OpenGrok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@tulinkry, I set only "Current version" to be fixed so the rest can adjust according to their size. I also fixed the extra margins. See new screenshots.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@amotzte thanks & sorry for stupid Q, this explains, I was under the impression this needs to be stored in history folder per every project, if we serialize it down to config. xml then great :) |
d971efa
to
5747523
Compare
@@ -125,12 +125,17 @@ Copyright (c) 2015, 2016, Oracle and/or its affiliates. All rights reserved. | |||
<% | |||
if (pHelper.getRepositories(group).size() > 0 ) { | |||
%> | |||
<table> | |||
<table > | |||
<col/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using <col>
as the attributes are mostly obsolete in html5
Extract this into the css files (default/style.css
, polished/style.css
, offwhite/style.css
). Do not forget to change all of them. You can set a class
attribute on the column or select it with some other more complicated selector.
@@ -125,12 +125,17 @@ Copyright (c) 2015, 2016, Oracle and/or its affiliates. All rights reserved. | |||
<% | |||
if (pHelper.getRepositories(group).size() > 0 ) { | |||
%> | |||
<table> | |||
<table > |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra space?
%> | ||
<span class="rev-message-summary"><%= coutSummary %></span> | ||
<span class="rev-message-full rev-message-hidden"><%= cout %></span> | ||
<div data-toggle-state="less"><a class="rev-toggle-a rev-message-toggle " href="#">show more ... </a></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it is in my screenshot...I'd make this <span>
instead of <div>
. Having an extra line seems unnecessarily to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<table> | ||
<thead> | ||
<table/> | ||
<col/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same problem like above about the css.
65514f5
to
39242da
Compare
Currently only support for git
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to one line and move col definition to css
%><td><%= Util.htmlize(type) %></td><% | ||
%><td><%= Util.htmlize(parent) %> (<%= Util.htmlize(branch) %>)</td><% | ||
%><td><% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
I'm fine |
@vegetablemao You might want to try the following snippet: String d = "Fri, 14 May 2010 23:24:24 +0200";
String one = "EE, d MMM yyyy HH:mm:ss Z";
String two = "d MMM yyyy HH:mm:ss Z";
SimpleDateFormat format = new SimpleDateFormat(one, Locale.getDefault());
try {
Date date = format.parse(d);
System.out.println("Successfully parsed " + date);
} catch (ParseException ex) {
System.out.println("Parsing failed");
} as this is the way we parse the git log dates. If it fails, try to change the line where the format is created to SimpleDateFormat format = new SimpleDateFormat(one); to see if it helps. |
@vegetablemao support for old git is already in the trunk (#1314)
as I added more verbose information to that exception. |
Why needed
Currently the repos table on home page already shows some details about the available projects. I found it very helpful to also show what is the current indexed version for each project. This way you can immediately know how much up to date the project is.
As a start I added the information for git SCM only but I prepared the infrastructure to add for other SCM (much like the “branch” column).
Here is the result: